Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
In case there are problems, just click a checkbox below to rerun the respective action.

• Re-run PSI checks
• Re-sync branch

Is this the only reference to set/getlibs? Don't we use them in tests too?

We do use set/getLibs in tests and other files which is why I left it as an export in the utils.js file and did not delete utils.js.

I don't think the removal of the import from scripts ought to effect anything, but Dennis should probably run a full smoke test.

Is this the only reference to set/getlibs? Don't we use them in tests too?

We do use set/getLibs in tests and other files which is why I left it as an export in the utils.js file and did not delete utils.js.

I don't think the removal of the import from scripts ought to effect anything, but Dennis should probably run a full smoke test.

Oh yeah. I didn't notice that you hadn't deleted utils.js. I assumed you were moving the function, not duplicating it. I don't understand how that works. Isn't libs scoped to that function, not on the window? I don't see how you can set libs in scripts.js and get it in utils.js or am I missing something...

Oh yeah. I didn't notice that you hadn't deleted utils.js. I assumed you were moving the function, not duplicating it. I don't understand how that works. Isn't libs scoped to that function, not on the window? I don't see how you can set libs in scripts.js and get it in utils.js or am I missing something...

In this instance, not so sure the closure matters since the function runs immediately and returns the get/set pattern and scripts looks for the global of window.location if a location is not provided to the first arrow function. We actually don't need libs for our utils.js, and we definitely don't need to make the call for our marquees. Before copying the libs pattern into scripts, we were essentially saying "scripts, go get bacom utils to figure out which path to load milo utils", now we're saying "scripts use setLibs which you have to figure out which utils to get". Were' cutting out the utils call here that blocks milo utils from running loadArea etc.

I left the function in utils because quite a bit of our code uses getLibs in order to use Milo core functionality: https://github.com/search?q=repo%3Aadobecom%2Fbacom%20getLibs&type=code

It's a bit over-engineered for what it does, we are passing a const and not using the second parameter or second function. If we're not changing prodlibs then this can all be deterministic with zero parameters or closures.

function getLibs() {
  const { hostname, search } = window.location;
  if (!(hostname.includes('.hlx.') || hostname.includes('local'))) return LIBS;
  const branch = new URLSearchParams(search).get('milolibs') || 'main';
  if (branch === 'local') return 'http://localhost:6456/libs';
  return branch.includes('--') ? `https://${branch}.hlx.live/libs` : `https://${branch}--milo--adobecom.hlx.live/libs`;
}

No need for a setLibs, at least in scripts.js. We could refactor utils.js later.
https://github.com/search?q=repo%3Aadobecom%2Fbacom+setLibs&type=code

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (stage@512d0a6). Learn more about missing BASE report.
Report is 4 commits behind head on stage.

@@           Coverage Diff            @@
##             stage     #209   +/-   ##
========================================
  Coverage         ?   96.28%           
========================================
  Files            ?       12           
  Lines            ?     1105           
  Branches         ?        0           
========================================
  Hits             ?     1064           
  Misses           ?       41           
  Partials         ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@JasonHowellSlavin Shouldn't this be working? https://utils-to-scripts--bacom--adobecom.hlx.page/drafts/methomas/tree-view

Also, what about the tests? If we don't need to set the libs anymore, can we remove that from the tests?

Also, what about the tests? If we don't need to set the libs anymore, can we remove that from the tests?

We need some tests here, this is too important not to test.

Also, what about the tests? If we don't need to set the libs anymore, can we remove that from the tests?

We need some tests here, this is too important not to test.

Agreed, but I think we're talking about two different things:

1. Like you said, we need unit tests for this. Maybe the tests from utils.test.js can be moved/adapted.
2. I'd also like us to clean up the block tests like tree-view.test.js that import and use setLibs from utils. If we're no longer using setLibs in the block code, then we don't need to have in the block tests, right?

Also, what about the tests? If we don't need to set the libs anymore, can we remove that from the tests?

We need some tests here, this is too important not to test.

Agreed, but I think we're talking about two different things:

1. Like you said, we need unit tests for this. Maybe the tests from utils.test.js can be moved/adapted.
2. I'd also like us to clean up the block tests like tree-view.test.js that import and use setLibs from utils. If we're no longer using setLibs in the block code, then we don't need to have in the block tests, right?

@Brandon32 , I will adapt the unit.test.js for this use case as Megan is saying, good point both.

For point 2. I think we need to look into that issue separately from this. There is some flakiness in our tests as they stand now, we may need an issue to look into them and refactor a bit. Right now many of the tests are throwing errors into the Browser logs, but are still passing, like tree-view, redirects-formatter, etc.

After discussing with team, and in light of my absence for a few weeks, we are closing this PR in favor of #220.